Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update weighting for the Aggregate score based on WEO2023 #327

Merged
merged 5 commits into from
May 30, 2024

Conversation

Antoine-Lalechere
Copy link
Contributor

@jdhoffa @MonikaFu
here are the lines of code to update the weighting for WEO2023 for the Aggregate Score.

Scenario raw data are stored on the following link in Azure but I don't know how to link Azure and GH, so the link used is on my machine for now - can you help me with it? https://portal.azure.com/#view/Microsoft_Azure_FileStorage/FileShareMenuBlade/~/browse/storageAccountId/%2Fsubscriptions%2Ffeef729b-4584-44af-a0f9-4827075512f9%2FresourceGroups%2FRMI-SP-PACTA-PROD%2Fproviders%2FMicrosoft.Storage%2FstorageAccounts%2Fpactarawdata/path/scenario-sources/protocol/SMB

Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 9.69%. Comparing base (b61c49d) to head (9740ede).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #327   +/-   ##
=====================================
  Coverage   9.69%   9.69%           
=====================================
  Files         27      27           
  Lines       2187    2187           
=====================================
  Hits         212     212           
  Misses      1975    1975           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MonikaFu
Copy link
Collaborator

thanks @Antoine-Lalechere. Reviewing now. If you want to use a path to azure storage instead of your local path, I don't think that this is possible. We could add a location of the file in a comment maybe. @jdhoffa do you have any input here?

@jdhoffa
Copy link
Member

jdhoffa commented May 28, 2024

More specifically, here:
https://pactarawdata.file.core.windows.net/scenario-sources/weo_2023-20240222/WEO2023 extended data/WEO2023_Extended_Data.xlsx

@MonikaFu
Copy link
Collaborator

The last link did not work for me @jdhoffa. Should it work in a browser? My experience with azure is that you can point to a directory but not a specific file. So you need to give a file location in the directory next to the link in the comment.

@MonikaFu
Copy link
Collaborator

In general, I would prefer that this dataset is an external dataset created in workflow.prep.* and passed to the relevant function as it will change with each COP and I don't think it makes much sense for it to be an internal dataset in this package. But given the time sensitivity I'd leave it as it is for now and add an issue for the next time we touch this repo (if we are going to be running more COP projects with executive summaries in the future).

Copy link
Collaborator

@MonikaFu MonikaFu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall works, I run it locally and got a correct plot. However, it would be preferred if the path to file is generalized, for example to "PATH/TO/SCENARIO/EXTENDED/DATA" or whatever other name would be the most accurate and then in the comment there was a link to azure where the file is located (potentially with the exact description of the location and name of the file as I am not sure if you can link directly to a file).

@MonikaFu
Copy link
Collaborator

Closes #318

@jdhoffa
Copy link
Member

jdhoffa commented May 29, 2024

The last link did not work for me @jdhoffa. Should it work in a browser? My experience with azure is that you can point to a directory but not a specific file. So you need to give a file location in the directory next to the link in the comment.

Hmm indeed, it doesn't work for me either that's odd... that is the URL i get as a result of "copy link", but anyway, the files are here:
https://portal.azure.com/#view/Microsoft_Azure_FileStorage/FileShareMenuBlade/~/browse/storageAccountId/%2Fsubscriptions%2Ffeef729b-4584-44af-a0f9-4827075512f9%2FresourceGroups%2FRMI-SP-PACTA-PROD%2Fproviders%2FMicrosoft.Storage%2FstorageAccounts%2Fpactarawdata/path/scenario-sources/protocol/SMB

and the specific file is: WEO2023_Extended_Data.xlsx

I suppose so long as those two pieces of information are tracked for posterity, we are good

@jdhoffa
Copy link
Member

jdhoffa commented May 29, 2024

@MonikaFu @Antoine-Lalechere I leave it to you two to decide exactly how you want to achieve that.
Agreed that for this project we shouldn't overthink where the data is stored, but probably in the long-term having it in workflow.* would make sense.

@MonikaFu MonikaFu requested a review from jdhoffa May 29, 2024 15:36
@MonikaFu
Copy link
Collaborator

MonikaFu commented May 29, 2024

@jdhoffa I requested a review from you since it was me who finished the PR in the end and I don't want to be reviewing my own code 🙃 FYI @Antoine-Lalechere

data-raw/remaining_carbon_budgets.R Outdated Show resolved Hide resolved
Comment on lines 29 to 34
carbon_emissions <- carbon_emissions[-5,]
carbon_emissions <- carbon_emissions[-5,]
carbon_emissions <- carbon_emissions[-5,]
carbon_emissions <- carbon_emissions[-5,]
carbon_emissions <- carbon_emissions[-5,]
carbon_emissions <- carbon_emissions[-5,]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a more readable/ interpretable way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image The issue is that those lines get the same labels

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this


remaining_carbon_budgets <- carbon_emissions %>%
mutate(
remaining_carbon_budget = emissions_2022 * 4 + emissions_2030 * 4, # remaining carbon dbusget is the interpolized carbon budget until 2030 for a sector
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we multiply by 4? Please add a comment to explain what is going on there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are searching the remaining carbon budget between 2022 and 2030, so we are interpolating the carbon budget from 2022 and 2030 and we multiply each one by 4 to account for the intermediate year (2023 to 2029)

Copy link
Collaborator

@MonikaFu MonikaFu May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Antoine-Lalechere do we include both 2022 and 2030 in the calculation? Or we start from 2023? (I assume the latter, otherwise we'd be missing one year) Do I understand correctly then that we assume emissions assigned to years 2023-2026 are assumed to be the same as those in 2022 and those for 2027-2030 are assumed to be equal to 2030? It is a bit of a strange interpolation tbh. Is there any reason why we do it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do an interpolation of carbon budget (so we have a line for carbon budget by sector between 2022 and 2030)
Then 2023 + 2029 carbon budget sums together and are equal to 2022 + 2030 carbon budget
same for 2024 + 2028 etc...
I think I forgot a year, it should be 4.5 and not 4 in the formula

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I understand. In the end after I run some calculations it seems that the sum of linearly interpolated points is the same as what you are doing here. The only unresolved question is if we should be using 2022 or not? Should we? Aren't we using data from 2023 to 2030 for the calculation?>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data we get is from 2022 so we have to consider it

Formula should actually be:

remaining_carbon_budget = emissions_2022 * 4.5 + emissions_2030 * 4.5,

This won't change the results as we take then percentage

@MonikaFu MonikaFu requested a review from jdhoffa May 30, 2024 14:40
@MonikaFu MonikaFu self-requested a review May 30, 2024 14:53
@MonikaFu MonikaFu merged commit a41ab4d into main May 30, 2024
9 checks passed
@MonikaFu MonikaFu deleted the 318-update-remaining_carbon-budget branch May 30, 2024 14:53
cjyetman pushed a commit to RMI-PACTA/workflow.transition.monitor that referenced this pull request Jun 10, 2024
Updating scenario source and scenario for executive summary
calculations. Aligned with what the team decided to use.

- [x] depends on
RMI-PACTA/pacta.executive.summary#325
- [x] depends on
RMI-PACTA/pacta.executive.summary#327

---------

Co-authored-by: Jackson Hoffart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants